Enhance param parse for 3L and LRB#204
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances parameter parsing in the LRB and 3LCache eviction modules by always applying default parameter values first and then overwriting them with user‐supplied values to avoid uninitialized objects. Key changes include:
- Applying default parameters in both LRB and 3LCache interfaces before user-specific parameters are parsed.
- Removing redundant default parsing in the else branch.
- Adding explicit freeing of the "objective" field in both the free functions and parse functions to prevent memory leaks.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libCacheSim/cache/eviction/LRB/LRB_Interface.cpp | Default parameters applied at init, with added freeing of "objective" in both the free and parse functions. |
| libCacheSim/cache/eviction/3LCache/ThreeLCache_Interface.cpp | Default parameters applied at init, and freeing of "objective" added in appropriate functions to ensure memory safety. |
Comments suppressed due to low confidence (2)
libCacheSim/cache/eviction/LRB/LRB_Interface.cpp:98
- Applying default parameters before parsing cache-specific parameters aligns with the PR’s objective; ensure these defaults are correctly overwritten if user parameters are provided.
LRB_parse_params(cache, DEFAULT_PARAMS);
libCacheSim/cache/eviction/3LCache/ThreeLCache_Interface.cpp:95
- Applying default parameters at the start ensures that all necessary fields are initialized; double-check that any cache-specific values subsequently provided correctly overwrite these defaults.
ThreeLCache_parse_params(cache, DEFAULT_PARAMS);
|
|
||
| if (strcasecmp(key, "objective") == 0) { | ||
| if (params->objective != NULL) { | ||
| free(params->objective); |
There was a problem hiding this comment.
Freeing the existing 'objective' before reassigning it avoids memory leaks; this practice is consistent with robust memory management.
| free(params->objective); | |
| free(params->objective); | |
| params->objective = NULL; |
| @@ -133,6 +132,9 @@ static void LRB_free(cache_t *cache) { | |||
| auto *LRB = static_cast<lrb::LRBCache *>(params->LRB_cache); | |||
| delete LRB; | |||
| free(cache->to_evict_candidate); | |||
There was a problem hiding this comment.
Including an explicit check and free for 'objective' in the free function helps prevent memory leaks; verify that the allocation lifecycle for 'objective' is consistently managed across code paths.
| free(cache->to_evict_candidate); | |
| free(cache->to_evict_candidate); | |
| // Ensure that 'objective' is freed to prevent memory leaks. |
| } | ||
|
|
||
| if (strcasecmp(key, "objective") == 0) { | ||
| if (params->objective != NULL) { |
There was a problem hiding this comment.
Properly freeing the existing 'objective' before assigning a new value prevents memory leaks; this update maintains consistency with similar patterns in the LRB module.
| static_cast<ThreeLCache::ThreeLCacheCache *>(params->ThreeLCache_cache); | ||
| delete ThreeLCache; | ||
| free(cache->to_evict_candidate); | ||
| if (params->objective != NULL) { |
There was a problem hiding this comment.
Adding a check and free for 'objective' in the free function reinforces memory safety; ensure this pattern is uniformly applied across all similar modules.
Provide default values initially, then overwrite them if parameters are supplied by the user. This prevents crashes from "misuse" such as passing empty strings, which would leave objects uninitialized in the current codebase.